Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add sd-streams from https://github.com/stardazed/sd-streams/blob/mast… #3192

Merged
merged 12 commits into from Oct 28, 2019

Conversation

nstott
Copy link
Contributor

@nstott nstott commented Oct 23, 2019

fixes #2828

@stardazed/streams was removed from third_party/ in https://github.com/denoland/deno/pull/2826/files

This adds sd streams from https://github.com/stardazed/sd-streams/tree/8928cf04b035fd02fb1340b7eb541c76be37e546 to cli/js/streams and reverts some of #2836

I've disabled some lints in cli/js/streams/ in order to get this to pass, I've marked them with TODOs,

As well, There are a few other TODO's that i've noted in streams/. notably, we're missing the DomException, and there are some gross type casts. as undefined as ..

There seems to be an impedance mismatch in the streams interface, eg, SDReadbleStream vs ReadableStream, but I'm not sure how much that matters at the moment

@hayd
Copy link
Contributor

hayd commented Oct 23, 2019

Should this be a git submodule rather than vendorize?

@nstott
Copy link
Contributor Author

nstott commented Oct 23, 2019

Should this be a git submodule rather than vendorize?

I was wondering this myself, but there are a bunch of changes that we need to make to the sd-source,
we need to bring in dom_types etc in various places (https://github.com/nstott/deno/blob/enable-sd-streams/cli/js/streams/pipe-to.ts#L17-L18), and we need to add the extension for all the imports (the source repo doesn't have those (https://github.com/stardazed/sd-streams/blob/master/packages/streams/src/pipe-to.ts#L8-L10).

@hayd
Copy link
Contributor

hayd commented Oct 23, 2019

Another option is to fork and float a patch, that way we could rebase over future updates...

getWriter(): WritableStreamDefaultWriter<W>;
}

export interface PipeOptions {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are called from cli/js/streams, and seem to be expected to exist in the dom.

I pulled most of these interfaces from https://github.com/microsoft/TypeScript/blob/master/lib/lib.dom.d.ts, I'm not sure whether this is right, or the right place to put these

@nstott nstott marked this pull request as ready for review October 23, 2019 20:29
@ry
Copy link
Member

ry commented Oct 27, 2019

@nstott I'd like to land this but it's red .. can you look into it?

@nstott
Copy link
Contributor Author

nstott commented Oct 27, 2019

will do!

@nstott
Copy link
Contributor Author

nstott commented Oct 27, 2019

That's done it.
I didn't realize these kinds were shared in both worlds.
I'm not sure this is the right place to put these.
There are a couple of places that sd-streams calls a DomException

https://github.com/stardazed/sd-streams/blob/45aa9d1ab8679cfcc3ca2959f6eef2276197d1c6/packages/streams/src/pipe-to.ts#L38

https://github.com/stardazed/sd-streams/blob/83df410ab4a0d1d049ecd3dccfefc9b36698a701/packages/streams/src/shared-internals.ts#L171

As far as I could tell, we didn't have an equivalent error type, so I shoved these into the DenoError bucket and punted

@nstott
Copy link
Contributor Author

nstott commented Oct 27, 2019

This is a lot of code that's being imported. If this is too big, I could stage these imports into smaller pieces.

for example, I could rip out the writer and transformer stuff from this pr, as we don't use those features yet, and don't have tests in place yet for that specific functionality.

@ry
Copy link
Member

ry commented Oct 28, 2019

FYI - this PR adds 1mb to the release executable on mac:

> ls -l target/release*/deno
-rwxr-xr-x  1 rld  staff  45396256 Oct 28 11:14 target/release-streams/deno
-rwxr-xr-x  2 rld  staff  44490880 Oct 28 11:16 target/release/deno

@ry ry mentioned this pull request Oct 28, 2019
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thank you !

@ry ry merged commit 65d9286 into denoland:master Oct 28, 2019
@nstott nstott deleted the enable-sd-streams branch October 28, 2019 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re-enable request.body tests
3 participants